@cderv I got no response from the original issue authors. Since my changes are minimal, I think they are compatible with the rest of Quarto and ready to be merged. Please let me know if there are other things I need to do.

PS: I signed a contribution agreement when I contributed to Positron, I think it also works for Quarto that I don't need to sign anything else?

Thanks for the PR.

I don't love an unbounded timeout loop; if something goes wrong we'll have something waking up the process at every 50ms, and that doesn't seem like a good idea.

What if we used a MutationObserver instead? We already use this in other parts of the Quarto codebase. Good docs here: https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver

@cscheid Just switched to MutationObserver, is the current version better?

Yes, I think so, thanks!

Other things we need to think about:

• how do we test this to avoid future regressions? We have some playwright tests on tests/integration/playwright that are controlled by tests/integration/playwright-tests.test.ts, but I'm not sure how easy it is to set those up with the giscus dependencies
• we need to check that the behavior stays correct for themes that lack light-dark
• You need to sign a contributor agreement with Posit: https://github.com/quarto-dev/quarto-cli/blob/main/CONTRIBUTING.md

Yes, I think so, thanks!

Other things we need to think about:

• how do we test this to avoid future regressions? We have some playwright.test.ts tests, but I'm not sure how easy it is to set those up with the giscus dependencies
• we need to check that the behavior stays correct for themes that lack light-dark
• You need to sign a contributor agreement with Posit: https://github.com/quarto-dev/quarto-cli/blob/main/CONTRIBUTING.md

As for the contributor agreement, I have signed it with Posit when I contributed to posit-dev/ark. I think it also works for Quarto?

As for websites without dark themes, I have browsed some websites without a dark theme in the Quarto gallery (e.g. https://nasa-openscapes.github.io/). It seems that for these websites, there is also a quarto-light class, which is sufficient for the mutation observer. The giscus is then set to giscus-base-theme if quart-dark is not detected. So in theory, this should work.

I'm not sure about how to avoid future regressions, either. I'm pretty new to TypeScript/JavaScript as you probably realised.

As for the contributor agreement, I have signed it with Posit when I contributed to posit-dev/ark. I think it also works for Quarto?

It does, thanks!

@cderv, asking for a review here specifically for guidance/idea around playwright.

I think we can indeed test this using our playwright testings. We just need to come up with an example website that has the issue, and for which this PR fixes it. Though, we also need to have a dummy giscus set up 🤔 - I'll look into this. @kv9898 I can handle adding the test and I'll probably push in your PR before merging

Though, we also need to have a dummy giscus set up 🤔 - I'll look into this.

Let's talk about this when you know more. As we start to become more diligent about integration tests, this kind of issue will come up more often.